Fixed cs migration template #1943
Conversation
| * | ||
| * @return void | ||
| */ | ||
| public function preUp(MigrationManager $manager): void |
There was a problem hiding this comment.
preUp() does not have to return anything, but if it returns false, migration will be aborted. So the PHP return type is probably mixed? Maybe psalm allows for something like false|void.
There was a problem hiding this comment.
so it is null|false|void in docblock it seems
Kind of shows why it is important to document those types :)
But kind of a code smell IMO legacy wise
Could we maybe use a different way to abort moving forward? Like using a specific exception that we catch?
There was a problem hiding this comment.
@michbeck Can we omit the actual type for now and just document as I suggested?
We can then in a follow up PR fix it up to use an exception based halting instead of false smell.
There was a problem hiding this comment.
Does psalm or stan complain if a method declares to throw an exception, but doesn't?
It would be a problem if this gives a warning:
/**
* @param \Propel\Generator\Manager\MigrationManager $manager
*
* @throws \...\AbortMigrationException
*
* @return void
*/
public function preUp(MigrationManager $manager)
{
// add the pre-migration code here
}
Codecov ReportBase: 88.69% // Head: 88.69% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #1943 +/- ##
=========================================
Coverage 88.69% 88.69%
Complexity 8002 8002
=========================================
Files 243 243
Lines 24474 24474
=========================================
Hits 21706 21706
Misses 2768 2768
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Follow up: Fix it using exception and void return. |
No description provided.